Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validate classes in config persistence #10484

Merged
merged 4 commits into from
Mar 26, 2022
Merged

Conversation

cgardens
Copy link
Contributor

What

One shortcoming of the ConfigPersistence iface is that it doesn't give great type safety. If you ask for a workspace object but query the sync table, it'll happily try to return this. The error handling on it isn't very clear. Sometimes it might fail because json validation will catch the mismatch. If the validation succeeds though, then you can end up with a weird, hard-to-debug output.

This is relevant as we think about consolidating StandardSourceDefinition and StandardDestinationDefinition into a single struct ActorDefinition. Making that change will be safer if we check that class we are asking for matches the class declare in the enum.

How

  • Add classname to AirbyteConfig. Build a decorate class that compares the classname in AirbyteConfig to class requested in getConfig(), etc, and the inputs into any write methods.

@cgardens cgardens temporarily deployed to more-secrets February 20, 2022 20:31 Inactive
@cgardens cgardens temporarily deployed to more-secrets February 20, 2022 20:31 Inactive
@cgardens cgardens temporarily deployed to more-secrets February 20, 2022 20:33 Inactive
@cgardens cgardens temporarily deployed to more-secrets February 20, 2022 20:33 Inactive
@github-actions github-actions bot added area/platform issues related to the platform area/server labels Feb 20, 2022
@cgardens cgardens temporarily deployed to more-secrets February 20, 2022 21:18 Inactive
@cgardens cgardens temporarily deployed to more-secrets February 20, 2022 21:18 Inactive
@cgardens cgardens temporarily deployed to more-secrets March 26, 2022 21:47 Inactive
@cgardens cgardens temporarily deployed to more-secrets March 26, 2022 21:47 Inactive
@cgardens cgardens merged commit 2de581d into master Mar 26, 2022
@cgardens cgardens deleted the cgardens/consolidate-defs branch March 26, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform area/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants